Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand support for EVP_PKEY_HMAC #1933

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

justsmth
Copy link
Contributor

@justsmth justsmth commented Oct 18, 2024

Issues:

Addresses CryptoAlg-2695

Description of changes:

  • Expands support for EVP_PKEY_HMAC
    • Support for "keygen" operations
    • Support for EVP_PKEY_CTX_ctrl_str.
    • Support for EVP_PKEY_CTRL_SET_MAC_KEY operation w/ EVP_PKEY_CTX_ctrl

Testing:

  • New tests added.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2024

Codecov Report

Attention: Patch coverage is 88.88889% with 13 lines in your changes missing coverage. Please review.

Project coverage is 78.68%. Comparing base (460a9dd) to head (9f6315e).

Files with missing lines Patch % Lines
crypto/fipsmodule/evp/p_hmac.c 79.36% 13 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1933   +/-   ##
=======================================
  Coverage   78.67%   78.68%           
=======================================
  Files         585      585           
  Lines      100849   100915   +66     
  Branches    14299    14312   +13     
=======================================
+ Hits        79347    79403   +56     
- Misses      20868    20875    +7     
- Partials      634      637    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justsmth justsmth force-pushed the ruby-hmac-integ branch 3 times, most recently from a1b36dd to c8d764b Compare October 23, 2024 15:29
@justsmth justsmth marked this pull request as ready for review October 23, 2024 15:39
@justsmth justsmth requested a review from a team as a code owner October 23, 2024 15:39
@justsmth justsmth changed the title [DRAFT] Expand support for EVP_PKEY_HMAC Expand support for EVP_PKEY_HMAC Oct 23, 2024
crypto/fipsmodule/evp/evp_ctx_test.cc Show resolved Hide resolved
crypto/fipsmodule/evp/p_hmac.c Outdated Show resolved Hide resolved
crypto/fipsmodule/evp/p_hmac.c Outdated Show resolved Hide resolved
crypto/fipsmodule/evp/p_hmac.c Show resolved Hide resolved
HMAC_KEY *hmac = NULL;
HMAC_PKEY_CTX *hctx = ctx->data;
if(hctx == NULL) {
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_PARAMETERS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: EVP_R_INVALID_PARAMETERS is more applicable to invalid ASN.1 inputs, EVP_R_OPERATON_NOT_INITIALIZED might be a bit better.

Suggested change
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_PARAMETERS);
OPENSSL_PUT_ERROR(EVP, EVP_R_OPERATON_NOT_INITIALIZED);

@@ -81,6 +81,10 @@ static int hmac_copy(EVP_PKEY_CTX *dst, EVP_PKEY_CTX *src) {
sctx = src->data;
dctx = dst->data;
dctx->md = sctx->md;
if(sctx->ktmp.key != NULL && !HMAC_KEY_copy(&sctx->ktmp, &dctx->ktmp)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to guard sctx->ktmp before accessing the internals?

Suggested change
if(sctx->ktmp.key != NULL && !HMAC_KEY_copy(&sctx->ktmp, &dctx->ktmp)) {
if(sctx->ktmp != NULL && sctx->ktmp.key != NULL && !HMAC_KEY_copy(&sctx->ktmp, &dctx->ktmp)) {

static const char *hmac_hexkey = "0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b0b";

TEST_F(EvpPkeyCtxCtrlStrTest, HMACKey) {
// Test Cases from RFC 5869.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RFC 5869 seems to be referring to HKDF?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants